Align with backchannel logout OIDC spec#1432
Align with backchannel logout OIDC spec#1432Spitfireap wants to merge 5 commits intonextcloud:mainfrom
Conversation
Spitfireap
force-pushed
the
fix/1430-backchannel-oidc-spec
branch
from
da506df to
149ded7
Compare
|
@julien-nc not entirely sure about the change for the error logging. |
Spitfireap
force-pushed
the
fix/1430-backchannel-oidc-spec
branch
2 times, most recently
from
0478e3e to
05b982d
Compare
|
Added some more alignment with the spec (exp + 1 iss validation step). Was wondering if we should verify that |
julien-nc
left a comment
julien-nc
left a comment
There was a problem hiding this comment.
Hey. #1431 is merged. Sorry there seems to be a small conflict with your branch. You can rebase on main or just merge main in your branch to resolve the conflict.
Was wondering if we should verify that jti and iat claims are there as well...
Yeah sure, let's keep it simple: It's enough to check if those token attributes are defined.
| ['iss_should_be_set' => true] | ||
| true | ||
| ); | ||
| } elseif ($iss !== $discovery['issuer']) { |
There was a problem hiding this comment.
$iss is not defined at this point.
| $this->logger->error('Backchannel logout error. ' . $error . ' ; ' . $description . | ||
| '. This is likely an IdP issue.'); |
There was a problem hiding this comment.
| $this->logger->error('Backchannel logout error. ' . $error . ' ; ' . $description . | |
| '. This is likely an IdP issue.'); | |
| $this->logger->error('Backchannel logout error. ' . $error . ' ; ' . $description . | |
| '. This is likely an IdP issue.', ['metadata' => $metadata]); |
You could keep the metadata param (which can be renamed) and include it in the logs so the information is more precise than the $isLikelyIdpSide boolean. Wdyt?
| ['multiple_sessions_found' => $sid] | ||
| ); | ||
| $this->logger->warning('[BackchannelLogout] Multiple OIDC sessions retrieved (sid+sub+iss). ' . | ||
| 'This should not happen. Please check that you have created your DB indexes') |
Working on May the 1st are you :p ? |
Checking your github notifications on May 1st? 😁 Yeah for a weird reason I'm working today. I can tell you on a private channel if you're curious.
Nice! Thank you for the enthusiasm. No rush though, feel free to take the public holiday 😉 |
refurbished $throttleMetadata not used since 9b5d6c6 Signed-off-by: Spitap <dev@asdrip.fr>
Signed-off-by: Spitap <dev@asdrip.fr>
Signed-off-by: Spitap <dev@asdrip.fr>
Signed-off-by: Spitap <dev@asdrip.fr>
Signed-off-by: Spitap <dev@asdrip.fr>
Spitfireap
force-pushed
the
fix/1430-backchannel-oidc-spec
branch
from
f0865f4 to
f209fb6
Compare
2 participants
fixes #1430
Current behaviour
Backchannel logout yields a HTTP/400 error when the session is not retrieved.
The spec states that it should be considered as a success and thus should return HTTP/200 :
Furthermore the response seems to be missing the Cache-Control header (spec).
The BC logout validation is also missing exp token validation (not expired) and iss validation (equals to issuer in discovery endpoint) per Backchannel logout token validation spec and ID Token validation spec
Changes
getBackchannelLogoutErrorResponse($throttleMetadata)